Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Owin instrumentation #156

Merged
merged 17 commits into from
Sep 29, 2021
Merged

Owin instrumentation #156

merged 17 commits into from
Sep 29, 2021

Conversation

CodeBlanch
Copy link
Member

@CodeBlanch CodeBlanch commented Sep 20, 2021 •
edited
Loading

Resolves #56

Changes

Added OpenTelemetry OWIN Instrumentation. Started from open-telemetry/opentelemetry-dotnet#1472 but made the following changes:

  • Moved to contrib
  • Dropped the netstandard2.0 target. NuGet was giving me errors about the version being restored for OWIN dependencies on netstandard2.0. I decided to just go with net461. I can't think of a really compelling reason to have netstandard2.0 and we can always add it later if needed.
  • Now uses ActivitySource
  • Merged the two projects into one
  • Added the usual options (Filter, Enrich, RecordException)
  • Added an example project

TODOs

  • Unit tests

@codecov
Copy link

codecov bot commented Sep 22, 2021 •
edited
Loading

Codecov Report

Merging #156 (050b29f) into main (f277cbe) will increase coverage by 0.15%.
The diff coverage is 80.67%.

❗ Current head 050b29f differs from pull request most recent head 09251a0. Consider uploading reports for the commit 09251a0 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main     #156      +/-   ##
==========================================
+ Coverage   78.62%   78.78%   +0.15%     
==========================================
  Files          79       85       +6     
  Lines        1984     2102     +118     
==========================================
+ Hits         1560     1656      +96     
- Misses        424      446      +22
Impacted Files Coverage Δ
...y.Contrib.Extensions.AWSXRay/AWSXRayIdGenerator.cs 64.19% <0.00%> (ø)
...n/Implementation/OwinInstrumentationEventSource.cs 26.31% <26.31%> (ø)
...umentation.Owin/TracerProviderBuilderExtensions.cs 83.33% <83.33%> (ø)
...ation.Owin/Implementation/DiagnosticsMiddleware.cs 91.86% <91.86%> (ø)
...ntrib.Instrumentation.Owin/AppBuilderExtensions.cs 100.00% <100.00%> (ø)
...mplementation/OwinInstrumentationActivitySource.cs 100.00% <100.00%> (ø)
...Instrumentation.Owin/OwinInstrumentationOptions.cs 100.00% <100.00%> (ø)

@CodeBlanch CodeBlanch marked this pull request as ready for review September 22, 2021 21:47
@CodeBlanch CodeBlanch requested a review from srprash as a code owner September 22, 2021 21:47
@CodeBlanch CodeBlanch requested a review from a team September 22, 2021 21:47
@CodeBlanch
Copy link
Member Author

@SergeyKanzhelev Any free cycles to do a review for me? Trying not to bug the guys working hard to finish up metrics support in SDK.

alanwest
alanwest approved these changes Sep 28, 2021
View reviewed changes
Copy link
Member

@alanwest alanwest left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Just one question about the Propagators option.

@CodeBlanch CodeBlanch merged commit 91a1269 into open-telemetry:main Sep 29, 2021
@CodeBlanch CodeBlanch deleted the owin-instrumentation branch September 29, 2021 05:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cijothomas cijothomas cijothomas left review comments

@alanwest alanwest alanwest approved these changes

@srprash srprash Awaiting requested review from srprash

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

OWIN Instrumentation
4 participants
@CodeBlanch @alanwest @cijothomas @denisivan0v